fix(terminal): use detected shell for execa and vscode terminal creation (#321)#333
fix(terminal): use detected shell for execa and vscode terminal creation (#321)#333F915 wants to merge 8 commits into
Conversation
…ion (Zoo-Code-Org#321) Connect the existing getShell() to both terminal creation paths to fix the Windows shell mismatch where AI generates pwsh syntax but commands execute in cmd.exe. - ExecaTerminalProcess: use getShell() instead of shell: true - Terminal: pass shellPath: getShell() to createTerminal Fixes Zoo-Code-Org#321
…gration (Zoo-Code-Org#321) Update two test cases that previously expected shell: true to now expect shell: expect.any(String), reflecting the change from shell: true to getShell() in ExecaTerminalProcess. Refs Zoo-Code-Org#321 Co-authored-by: Zoo Code Contributor <contributor@zoocode.dev>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughShell resolution now prefers ChangesTerminal shell & integration changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
F915
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8f5fb7a1-993e-4d1d-987f-4466eb7afe09
📒 Files selected for processing (3)
src/integrations/terminal/ExecaTerminalProcess.tssrc/integrations/terminal/Terminal.tssrc/integrations/terminal/__tests__/ExecaTerminalProcess.spec.ts
Mock getShell() to verify the fallback actually uses the detected shell instead of just checking for any string value. Suggested-by: CodeRabbit
…ll() integration (Zoo-Code-Org#321) PR Zoo-Code-Org#333 added shellPath and iconPath to Terminal constructor but TerminalRegistry.spec.ts assertions were not updated, causing 4 test failures on ubuntu-latest CI.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/integrations/terminal/__tests__/TerminalRegistry.spec.ts (1)
39-40: ⚡ Quick winAssert
getShell()invocation to prevent hardcoded-shell regressions.You already assert the resulting
shellPath, but addingexpect(shellUtils.getShell).toHaveBeenCalled()makes this test fail if production code hardcodes the same string instead of calling the resolver.Suggested test hardening
it("creates terminal with PAGER set appropriately for platform", () => { TerminalRegistry.createTerminal("/test/path", "vscode") + expect(shellUtils.getShell).toHaveBeenCalledTimes(1) expect(mockCreateTerminal).toHaveBeenCalledWith({ cwd: "/test/path", name: "Roo Code", iconPath: expect.objectContaining({ id: expect.any(String) }), env: { PAGER, ROO_ACTIVE: "true", VTE_VERSION: "0", PROMPT_EOL_MARK: "", }, shellPath: "/mock/fallback-shell", }) })Also applies to: 60-61, 83-84, 107-108, 130-131
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/integrations/terminal/__tests__/TerminalRegistry.spec.ts` around lines 39 - 40, Add assertions that the shell resolver is actually invoked: after each vi.spyOn(shellUtils, "getShell").mockReturnValue("/mock/fallback-shell") in the TerminalRegistry.spec tests, add expect(shellUtils.getShell).toHaveBeenCalled() (or toHaveBeenCalledTimes(1)) to ensure production code calls shellUtils.getShell rather than hardcoding the path; apply this to the occurrences around the tests that set up mocked shells (the blocks using vi.spyOn(shellUtils, "getShell") and asserting shellPath).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/integrations/terminal/__tests__/TerminalRegistry.spec.ts`:
- Around line 39-40: Add assertions that the shell resolver is actually invoked:
after each vi.spyOn(shellUtils,
"getShell").mockReturnValue("/mock/fallback-shell") in the TerminalRegistry.spec
tests, add expect(shellUtils.getShell).toHaveBeenCalled() (or
toHaveBeenCalledTimes(1)) to ensure production code calls shellUtils.getShell
rather than hardcoding the path; apply this to the occurrences around the tests
that set up mocked shells (the blocks using vi.spyOn(shellUtils, "getShell") and
asserting shellPath).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 11da761d-d24d-4448-bec2-2eb4363d25fa
📒 Files selected for processing (1)
src/integrations/terminal/__tests__/TerminalRegistry.spec.ts
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
proyectoauraorg
left a comment
proyectoauraorg
left a comment
There was a problem hiding this comment.
LGTM! Using the detected shell for both execa and VS Code terminal creation resolves the shell mismatch issue on Windows. Clean fix.
| const env = Terminal.getEnv() | ||
| const iconPath = new vscode.ThemeIcon("rocket") | ||
| this.terminal = terminal ?? vscode.window.createTerminal({ cwd, name: "Roo Code", iconPath, env }) | ||
| this.terminal = terminal ?? vscode.window.createTerminal({ cwd, name: "Roo Code", iconPath, env, shellPath: getShell() }) |
There was a problem hiding this comment.
If the user has WSL configured but no explicit shell path, getShell() returns /bin/bash (a Unix path) — does passing that as shellPath actually work on Windows, or would wsl.exe be needed here?
There was a problem hiding this comment.
Good catch, and you're right that this was a real issue.
After further analysis, we've addressed this in the follow-up refactoring (commit 387ca89). The terminal constructor now checks for WSL configuration first — when WSL is detected, shellPath is omitted so VS Code's own WSL profile handles shell selection. Passing an explicit shell path would bypass VS Code's profile system.
With this fix, the inline terminal path can now correctly execute WSL commands. However, the external terminal path still cannot work with WSL, because VS Code shell integration does not support WSL — the supported shells on Windows are limited to Git Bash and pwsh. In practice, the external terminal will wait for shell integration until timeout, then fall back to the inline terminal — which is the expected behavior.
These changes are part of a broader consolidation of shell detection that the WSL issue prompted us to revisit. The upstream code manually parsed VS Code's terminal profile configuration with a shell allowlist, but this approach was inherently fragile — it could not reliably match VS Code's internal shell resolution, which is exactly the kind of mismatch your comment pointed out. To address this at the root, we removed the allowlist and manual parsing in favor of vscode.env.shell, the API VS Code has provided for this purpose. As a result, the two command execution paths now share the same shell resolution logic, so there is no longer a risk of falling back to a different shell than the one the external terminal used. A safety check that guarded against this mismatch — but relied on the same fragile detection it was meant to protect against — became redundant and was removed.
We also removed a sendText call that previously fired before the fallback when shell integration was unavailable — this could cause the command to execute in the external terminal without output capture, and then again via the inline path, resulting in unintended double execution. The fallback to the inline terminal now happens directly.
Thanks again for taking the time to review — the feedback was helpful in tracing this issue to its root cause.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/integrations/terminal/__tests__/ExecaTerminalProcess.spec.ts (1)
176-212: 💤 Low valueUnusual semicolon-prefixed statement formatting.
Lines 180, 189, 198, and 207 use a semicolon prefix before calling
trimRetrievedOutput(). This is syntactically valid but unconventional and reduces readability.♻️ Cleaner formatting
it("clears buffer when all output has been retrieved", () => { terminalProcess["fullOutput"] = "test output data" terminalProcess["lastRetrievedIndex"] = 16 - ; (terminalProcess as any).trimRetrievedOutput() + ;(terminalProcess as any).trimRetrievedOutput() expect(terminalProcess["fullOutput"]).toBe("")Or wrap the call to avoid ASI ambiguity:
- terminalProcess["lastRetrievedIndex"] = 16 - ; (terminalProcess as any).trimRetrievedOutput() + terminalProcess["lastRetrievedIndex"] = 16 + const instance = terminalProcess as any + instance.trimRetrievedOutput()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/integrations/terminal/__tests__/ExecaTerminalProcess.spec.ts` around lines 176 - 212, The tests use unconventional semicolon-prefixed calls like ;(terminalProcess as any).trimRetrievedOutput() which is unnecessary and harms readability; update each test to call the method directly (e.g., (terminalProcess as any).trimRetrievedOutput()) or otherwise wrap the expression to avoid the leading semicolon—modify the four occurrences that invoke trimRetrievedOutput on the terminalProcess instance to remove the prefixed semicolons.src/utils/__tests__/shell.spec.ts (1)
203-263: 💤 Low valueConsider restoring the original
getConfigurationmock after eachgetWslProfiletest.The tests directly reassign
vscode.workspace.getConfigurationwithout restoring it, which could cause test pollution if tests run in a different order or if new tests are added to this describe block.♻️ Suggested improvement
describe("getWslProfile()", () => { + let originalGetConfiguration: typeof vscode.workspace.getConfiguration + beforeEach(() => { Object.defineProperty(process, "platform", { value: "win32" }) + originalGetConfiguration = vscode.workspace.getConfiguration + }) + + afterEach(() => { + vscode.workspace.getConfiguration = originalGetConfiguration })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/__tests__/shell.spec.ts` around lines 203 - 263, The tests for getWslProfile directly overwrite vscode.workspace.getConfiguration and never restore it, risking test pollution; update the describe("getWslProfile") block to save the original getConfiguration (e.g., const originalGetConfig = vscode.workspace.getConfiguration) in a beforeEach and restore it in an afterEach (or use jest.spyOn(vscode.workspace, "getConfiguration") and call mockRestore() in afterEach) so each it(...) can safely mock getConfiguration without leaking state across tests.src/integrations/terminal/ExecaTerminalProcess.ts (1)
37-88: 💤 Low value
effectiveCommandis assigned but unused in the WSL branch.The variable
effectiveCommandon line 47 is only used in the non-WSL else branch (line 87), but it's assigned regardless. This is minor dead code whenisWslShellis true.♻️ Suggested cleanup
const resolvedShell = BaseTerminal.getExecaShellPath() || getShell() const isWslShell = resolvedShell === WSL_EXE_PATH - let effectiveShell: string | boolean = resolvedShell - let effectiveCommand = command - if (isWslShell) { // Spawn wsl.exe directly (not through cmd.exe) to avoid nested-quoting issues. // execa(file, args, options) passes args as an array — no shell interpretation. @@ ... @@ } else { this.subprocess = execa({ - shell: effectiveShell, + shell: resolvedShell, cwd: this.terminal.getCurrentWorkingDirectory(), all: true, stdin: "ignore", env: { ...process.env, LANG: "en_US.UTF-8", LC_ALL: "en_US.UTF-8", }, - })`${effectiveCommand}` + })`${command}` }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/integrations/terminal/ExecaTerminalProcess.ts` around lines 37 - 88, The run method assigns effectiveCommand but never uses it in the WSL branch; remove the dead assignment by either moving the line that sets effectiveCommand into the non-WSL else branch or stop using effectiveCommand entirely and pass command directly to the non-WSL execa template literal—update the run function (referencing run, effectiveCommand, isWslShell, and the execa calls) so effectiveCommand is only created/used where needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/integrations/terminal/__tests__/TerminalProcessExec.cmd.spec.ts`:
- Around line 52-54: The env object literal sets shell to
"C:\Windows\System32\cmd.exe" with unescaped backslashes; update the shell
string in the test (the env object, shell property) to use escaped backslashes
("C:\\Windows\\System32\\cmd.exe") so the path is a valid JavaScript string
literal and avoids accidental escape sequences.
In `@src/integrations/terminal/__tests__/TerminalProcessExec.pwsh.spec.ts`:
- Around line 52-54: The Windows path string assigned to env.shell currently
uses single backslashes which are interpreted as escape sequences; update the
test to use escaped backslashes (e.g., double backslashes) in the "shell" value
so the path reads correctly (replace "C:\Program Files\PowerShell�\pwsh.exe"
with an escaped form); locate the env object in the
TerminalProcessExec.pwsh.spec.ts test and change the shell string to use "\\"
for each backslash.
---
Nitpick comments:
In `@src/integrations/terminal/__tests__/ExecaTerminalProcess.spec.ts`:
- Around line 176-212: The tests use unconventional semicolon-prefixed calls
like ;(terminalProcess as any).trimRetrievedOutput() which is unnecessary and
harms readability; update each test to call the method directly (e.g.,
(terminalProcess as any).trimRetrievedOutput()) or otherwise wrap the expression
to avoid the leading semicolon—modify the four occurrences that invoke
trimRetrievedOutput on the terminalProcess instance to remove the prefixed
semicolons.
In `@src/integrations/terminal/ExecaTerminalProcess.ts`:
- Around line 37-88: The run method assigns effectiveCommand but never uses it
in the WSL branch; remove the dead assignment by either moving the line that
sets effectiveCommand into the non-WSL else branch or stop using
effectiveCommand entirely and pass command directly to the non-WSL execa
template literal—update the run function (referencing run, effectiveCommand,
isWslShell, and the execa calls) so effectiveCommand is only created/used where
needed.
In `@src/utils/__tests__/shell.spec.ts`:
- Around line 203-263: The tests for getWslProfile directly overwrite
vscode.workspace.getConfiguration and never restore it, risking test pollution;
update the describe("getWslProfile") block to save the original getConfiguration
(e.g., const originalGetConfig = vscode.workspace.getConfiguration) in a
beforeEach and restore it in an afterEach (or use jest.spyOn(vscode.workspace,
"getConfiguration") and call mockRestore() in afterEach) so each it(...) can
safely mock getConfiguration without leaking state across tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dff403bf-4ce8-4795-9eca-970a7a523af7
📒 Files selected for processing (13)
src/__mocks__/vscode.jssrc/integrations/terminal/ExecaTerminalProcess.tssrc/integrations/terminal/Terminal.tssrc/integrations/terminal/TerminalProcess.tssrc/integrations/terminal/__tests__/ExecaTerminalProcess.spec.tssrc/integrations/terminal/__tests__/TerminalProcess.spec.tssrc/integrations/terminal/__tests__/TerminalProcess.test.tssrc/integrations/terminal/__tests__/TerminalProcessExec.bash.spec.tssrc/integrations/terminal/__tests__/TerminalProcessExec.cmd.spec.tssrc/integrations/terminal/__tests__/TerminalProcessExec.pwsh.spec.tssrc/integrations/terminal/__tests__/TerminalRegistry.spec.tssrc/utils/__tests__/shell.spec.tssrc/utils/shell.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/integrations/terminal/tests/TerminalRegistry.spec.ts
…-Org#321) Replace manual VS Code config parsing and allowlist validation in shell.ts with vscode.env.shell, the API VS Code provides for extension shell detection since version 1.37. - shell.ts: remove SHELL_ALLOWLIST (~95 entries) and manual config parsing functions (getWindowsShellFromVSCode, getMacShellFromVSCode, getLinuxShellFromVSCode). getShell() reads vscode.env.shell directly, falling back through os.userInfo(), COMSPEC/SHELL environment variables, and platform defaults. Remove getShellFallbackOccurred(). - Terminal.ts: construct terminal without explicit shellPath when the user has not set execaShellPath or configured WSL, letting VS Code's profile system determine the shell. When execaShellPath is set, pass it explicitly. For WSL, omit shellPath so VS Code uses its WSL profile for shell integration. Start the shell-integration-ready promise in the constructor. - TerminalProcess.ts: remove the getShellFallbackOccurred check that previously guarded against mismatch between the terminal's shell and the detected shell. After the shell.ts refactoring, both paths use the same VS Code resolution. When shell integration is unavailable, emit no_shell_integration without calling sendText first. In the stream processing path, treat missing OSC 633/133 markers as complete output instead of an error. - ExecaTerminalProcess.ts: use BaseTerminal.getExecaShellPath() || getShell() to match Terminal's shell resolution. When the shell is wsl.exe, spawn it directly with array arguments instead of wrapping through a shell. - tests: update shell.spec.ts for the new getShell() fallback chain. Update terminal test files to mock vscode.env.shell instead of stubbing getShell(). Extend vscode.js mock with onDidChangeTerminalShellIntegration event and env.shell. Update TerminalProcess.spec.ts to verify sendText is not called before no_shell_integration (preventing double execution). Update TerminalRegistry.spec.ts to drop the getShell spy and shellPath assertions now that Terminal no longer passes an explicit shellPath by default. Add WSL direct-spawn test in ExecaTerminalProcess.spec.ts. Fixes Zoo-Code-Org#321
3 participants
Related GitHub Issue
Closes: #321
Description
Zoo Code correctly detects the user's configured default shell (e.g., PowerShell) and tells the AI to generate commands using that shell's syntax, but the Inline Terminal actually executed commands in
cmd.exe. This caused:Write-Host) to fail with "not recognized" errorscmd.exeencodes output using the system codepage (e.g., CP936 for zh-CN) rather than UTF-8This PR connects shell detection to both terminal execution paths so the user's configured shell is used consistently.
Phase 1 — Connect shell detection to execution paths
Core changes (2 files):
ExecaTerminalProcess.ts — Changed
shell: truetoshell: getShell(). The detected shell from VS Code'sterminal.integrated.defaultProfileis now passed to execa subprocess execution.Terminal.ts — Added
shellPath: getShell()tovscode.window.createTerminal(). Also addediconPath: new vscode.ThemeIcon("rocket").Test updates (2 files):
ExecaTerminalProcess.spec.ts — Assertions updated from
shell: truetoshell: "/mock/fallback-shell"usingvitest.spyOnpattern.TerminalRegistry.spec.ts — Assertions updated to include the new
shellPathparameter.Phase 2 — Consolidate shell detection and fix WSL support
After review feedback from @edelauna, we identified that passing
getShell()asshellPathon Windows with WSL configured was problematic —getShell()could return a Unix path (/bin/bash) that isn't valid as a WindowsshellPath. Further analysis revealed deeper issues: the existing shell detection approach (manual VS Code config parsing + allowlist) could diverge from VS Code's internal shell resolution, creating a mismatch between the external terminal's shell and the fallback shell.5. shell.ts — Removed
SHELL_ALLOWLIST(~95 entries) and manual config parsing functions (getWindowsShellFromVSCode,getMacShellFromVSCode,getLinuxShellFromVSCode).getShell()now readsvscode.env.shelldirectly (the API VS Code has provided since 1.37), falling back throughos.userInfo(),COMSPEC/SHELLenvironment variables, and platform defaults. RemovedgetShellFallbackOccurred(). AddedWSL_EXE_PATHexport for canonical WSL path comparison.6. Terminal.ts — Three-branch constructor replaces the single
shellPath: getShell()call:getWslProfile()): omitshellPathentirely — lets VS Code's WSL profile handle shell selection and PTY bridge setup.execaShellPathset: pass explicitshellPathas the user configured.shellPath— defers to VS Code's profile system.The
shellIntegrationReadypromise is now started in the constructor (event-based detection + polling + timeout), reducing per-command wait time.7. TerminalProcess.ts — Three corrections to the fallback path:
getShellFallbackOccurred()check. This check guarded against a mismatch between the terminal's shell and the detected shell, but after theshell.tsrefactoring both paths use the same VS Code resolution — the mismatch scenario no longer exists.sendTextcall that previously fired before theno_shell_integrationfallback. When shell integration was unavailable, the code would callterminal.sendText(command)(executing the command in the external terminal without output capture), then emitno_shell_integration(triggering execa re-execution) — resulting in unintended double execution. The fallback now emitsno_shell_integrationdirectly.8. ExecaTerminalProcess.ts — Uses
BaseTerminal.getExecaShellPath() || getShell()for unified shell resolution, matching Terminal.ts. When the resolved shell iswsl.exe, spawns it directly with array arguments (--cd <wsl-path> -- bash -c <command>) instead of wrapping through a shell, avoiding nested-quoting issues. Path conversion handles Windows drive letters →/mnt/<drive>/mapping.9. Tests — Updated shell.spec.ts for the new
getShell()fallback chain. Updated TerminalProcess.spec.ts, TerminalProcess.test.ts, TerminalRegistry.spec.ts, ExecaTerminalProcess.spec.ts, and platform-specific TerminalProcessExec test files to match new behavior. Extendedvscode.jsmock withonDidChangeTerminalShellIntegrationevent andenv.shell. Fixed backslash escape issues in Windows test paths (\W,\S,\c,\P,\7are interpreted as escape sequences in JavaScript string literals). Added WSL direct-spawn test in ExecaTerminalProcess.spec.ts.Known limitation
WSL terminals do not support VS Code shell integration — the supported shells on Windows are limited to Git Bash and pwsh. The OSC 633/133 sequences that shell integration relies on don't traverse the WSL PTY bridge reliably. In practice, the external terminal will wait for shell integration until timeout, then fall back to the inline terminal (execa) — this is expected behavior. The inline terminal path correctly executes WSL commands via direct
wsl.exespawn.Test Procedure
Unit tests:
Type checking:
Manual verification (Windows with PowerShell configured):
Prerequisites: PowerShell configured with UTF-8 encoding.
Write-Host "test"in Zoo Code Inline Terminal'Write-Host' is not recognized as an internal or external commandtestecho "中文测试"in Zoo Code Inline Terminal中文测试(PowerShell with UTF-8 configured)WSL verification (Windows with WSL configured as default profile):
wsl.exewithout nested-quoting errors.Pre-Submission Checklist
Screenshots / Video
N/A — backend fix with no visible UI changes.
Documentation Updates
The shell integration docs need the following updates:
terminal.integrated.defaultProfilesetting, rather than always defaulting tocmd.exe(Windows) orsh(Unix).wsl.exespawn.cmd.exeencodes output using the system codepage rather than UTF-8. Running in the configured shell (e.g., PowerShell) enables easier UTF-8 configuration, though explicit setup is still required.Additional Notes
None.
Get in Touch
Discord: Yon (yon.sinc)